Skip to content

fix(proxy): clear expired retry cooldown transitions - #1908

Closed
JustYannicc wants to merge 4 commits into
Soju06:mainfrom
JustYannicc:fix/half-open-cooldown-beta4
Closed

fix(proxy): clear expired retry cooldown transitions#1908
JustYannicc wants to merge 4 commits into
Soju06:mainfrom
JustYannicc:fix/half-open-cooldown-beta4

Conversation

@JustYannicc

@JustYannicc JustYannicc commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem

An absent or elapsed durable retry cooldown was converted into now_monotonic + 0. Admission interpreted that value as a real expiry transition and could create a half-open lease when no active cooldown existed.

Why this PR exists

#1908 identified the arithmetic bug while the broader retry and stale-anchor work was being split. The root problem is real, but this branch also clears active half-open leases on equal or newer expired snapshots. That breaks the one-probe single-flight behavior introduced for #1394.

How this PR solves it

  • Normalize elapsed durable cooldowns during retry-circuit load.
  • Clear stale local cooldown and half-open deadlines on equal-version reload.
  • Add retry-circuit regressions for elapsed rows and lookup failure.

OpenSpec: openspec/changes/normalize-expired-retry-cooldown/

Scope

This branch changes retry-circuit normalization only. It does not own cooldown-suppressed session retirement from #1947, poisoned-anchor quarantine from #1891, or broad stale-anchor recovery from #1867.

Verification

Exact candidate: ed8ee1222999bf6b58164529af3ae5724e09c4ec

  • 37 retry-circuit tests passed.
  • Ruff, formatting, ty, proxy architecture, strict targeted OpenSpec, and git diff --check passed on the historical candidate.

Current status

Do not merge this branch. The zero-sentinel normalization is being rebuilt on current upstream main in a focused successor that preserves the active owner lease, admits exactly one probe after genuine expiry, and settles only the matching owner and token. #1908 will be superseded and closed after that successor passes exact-head local and hosted review.

A persisted retry-circuit row with an elapsed (or absent) cooldown currently reloads its deadline as a non-zero monotonic timestamp in the past. The admission check interprets that state as a cooldown that just ended, consumes the exclusive half-open lease, and suppresses subsequent requests for the lease duration even though no cooldown remains. Normalize non-positive remaining durable cooldowns to the zero sentinel while preserving future deadlines. Add a regression proving elapsed rows do not burn a lease; thresholds, backoff, persistence, and ownership behavior remain unchanged.
@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6a9026a6-70c2-4946-84dd-f428862e5d13

📥 Commits

Reviewing files that changed from the base of the PR and between b311aea and 32cf3aa.

📒 Files selected for processing (6)
  • app/modules/proxy/_service/http_bridge/retry_circuit.py
  • openspec/changes/normalize-expired-retry-cooldown/.openspec.yaml
  • openspec/changes/normalize-expired-retry-cooldown/proposal.md
  • openspec/changes/normalize-expired-retry-cooldown/specs/responses-api-compat/spec.md
  • openspec/changes/normalize-expired-retry-cooldown/tasks.md
  • tests/unit/test_proxy_http_bridge.py

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The retry circuit now restores elapsed or absent durable cooldowns as an open state with no cooldown or half-open lease. Equal-version reloads clear stale local deadlines. Specifications and unit tests document and verify the behavior.

Changes

Retry cooldown normalization

Layer / File(s) Summary
Cooldown normalization contract
openspec/changes/normalize-expired-retry-cooldown/*
The specification defines zero cooldown handling for absent or elapsed durable cooldowns. Future cooldowns remain enforced.
Retry-circuit state reconciliation
app/modules/proxy/_service/http_bridge/retry_circuit.py
State loading maps elapsed cooldowns to 0.0. Equal-or-newer records without active cooldowns clear stale local deadlines.
Cooldown expiry regression coverage
tests/unit/test_proxy_http_bridge.py
Tests verify expired cooldown restoration, expiry cleanup, repeated admission, and the absence of unnecessary half-open leases.

Estimated code review effort: 2 (Simple) | ~15 minutes

Merge Risk: ⚪ Minimal · up to 32cf3

This localized change clears expired retry cooldown state while preserving newer failures and persisted state; the supplied checks pass, and no actionable merge-blocking risk remains beyond normal review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: clearing expired retry-circuit cooldown transitions.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@JustYannicc

Copy link
Copy Markdown
Contributor Author

Follow-up on the expiry-transition review: commit ed8ee12 also clears a stale half-open lease when an equal-or-newer expired durable snapshot arrives after a transient lookup failure. Added regression coverage for future row -> lookup failure/lease -> same-version expired row. The branch is based directly on beta.4 main b311aea; no #1867 dependency. Affected gates pass: 37 retry-circuit tests, Ruff, format, ty, architecture, diff check, and strict scoped OpenSpec.

@Soju06

Soju06 commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Thanks — the underlying bug here is real and your regression tests demonstrate it well: an elapsed durable cooldown reloading as now_monotonic + 0 is nonzero-but-expired, so _http_bridge_precreated_retry_allowed treats every reload as a fresh "cooldown just ended" transition and burns a 600s half-open lease, suppressing all other requests on the hard key. A few things block this as-is:

  1. CI: the required "Contributors attribution" check fails — please add yourself to .all-contributorsrc (this is what turns "CI Required" red; everything substantive is green).

  2. Scope of the half-open clearing: the load normalization (persisted_cooldown_until = ... if cooldown_remaining > 0.0 else 0.0) fixes the misclassification cleanly. But the added equal-/newer-version branches that also zero half_open_until change the design: because the durable row is refreshed before every admission decision, any genuinely expired cooldown is observed via reload, so after expiry no half-open lease is ever taken and all concurrent requests are admitted simultaneously against an anchor with consecutive_failures >= 2 (your tests test_..._expiry_clears_loaded_local_deadline and test_..._expiry_clears_lookup_failure_probe assert exactly this). That effectively retires the single-flight probe from fix(http-bridge): stabilize silent and clean-close recovery #1394 except during durable-lookup-failure windows, and weakens the input signal the poison-anchor detection relies on. Could you either narrow the clearing (e.g., only for rows that never carried a real cooldown — the below-threshold now_wall writes — or only release leases whose probe never dispatched) or make the case explicitly in the OpenSpec delta that single-flight probing after expiry is being intentionally dropped, so the owner can rule on it?

  3. Overlap with fix(proxy): stop hard bridge keys wedging on a leaked half-open probe and a rejected continuity anchor #1857: the normalization hunk, including the comment text, is identical to the one in fix(proxy): stop hard bridge keys wedging on a leaked half-open probe and a rejected continuity anchor #1857 (opened Aug 20), which addresses the same leaked-half-open-probe wedge while keeping single-flight semantics via an owner-tracked lease release. Please coordinate — either rebase this as a scoped extraction with credit, or fold your equal-version-expiry observations into that PR's review.

  4. fix(proxy): complete stale-anchor recovery hardening #1867 collision: your own fix(proxy): complete stale-anchor recovery hardening #1867 rewrites the same load/merge path in retry_circuit.py. The branches are git-independent, but semantically one will need reworking after the other lands — worth stating which you'd prefer merged first.

@JustYannicc

Copy link
Copy Markdown
Contributor Author

Closing this branch unmerged because #1962 supersedes its accepted scope on current main. #1962 keeps the cooldown normalization and exact owner-bound half-open probe release, preserves the #1394 single-flight behavior, adds cancellation-safe teardown and OpenSpec coverage, and deliberately drops this PRs rejected equal/newer lease clearing. Exact successor head: eadaf45. Its local and independent reviews are complete and CodeRabbit has no current actionable threads. Hosted CI and Simplicity approval plus human maintainer review remain pending, so this is a supersession statement, not a merge-ready or deployment claim. No container mutation was performed.

@JustYannicc

Copy link
Copy Markdown
Contributor Author

Correction to the supersession record: #1962 has since advanced from eadaf456 to exact current head 6fb97330d8a73fa08c587edd7aedb90e1d5be039 (tree 0e3973601a7433e259db733d71ac20301085a8c3). The closure and scope decision are unchanged: #1908 remains closed unmerged; #1962 is the current-main successor. Hosted workflow approval and maintainer review remain external gates; no deployment claim.

@JustYannicc

JustYannicc commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Correction to the supersession record: #1962 has advanced to exact current head 6059522a58724e00d6a24433b7e183f5a6935a05 (tree 9419cd9b0f8fd60033063df70579cb42be1ac8b9), based on upstream main / merge-base 02113fd980623c791da943eefb7d3f4c9b838aaf. #1908 remains closed unmerged; #1962 is the current-main successor for the accepted cooldown-normalization and owner-bound probe scope. Hosted CodeRabbit is running on this exact head; workflow approval and maintainer review remain external gates. No deployment claim.

@JustYannicc

Copy link
Copy Markdown
Contributor Author

Final correction to the supersession record: #1962 now points to exact current-main head 3804ecd56d0b00b32d5081687e67777fa0250b19 (tree 8e6b8b7942475a069481c4599b1c2d448e7a0c46), with fresh exact-head CodeRabbit and independent Standards reviews passing. The closure and scope decision are unchanged: #1908 remains closed unmerged, and #1962 is the current-main successor for the accepted cooldown-normalization and owner-bound probe scope. Hosted CI/Simplicity still require repository-admin approval; no merge or deployment claim.

@JustYannicc

Copy link
Copy Markdown
Contributor Author

Final supersession record correction: #1962 is now at exact current-main head 3847c26a306821c9da543d921f95db90e5bca0d6 (tree 9b0d3daf18954c8cac62f251acead2f14d01e4ae, merge-base 02113fd980623c791da943eefb7d3f4c9b838aaf). #1908 remains closed unmerged; #1962 is the current-main successor for the accepted cooldown-normalization and owner-bound probe scope. Current-head CodeRabbit/Input/Standards evidence is recorded on #1962; hosted workflow approval and maintainer review remain external gates. No deployment claim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants